Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Identity] Fix PowershellCredential token parsing logic #30508

Merged
merged 11 commits into from
Jul 26, 2024

Conversation

KarishmaGhiya
Copy link
Member

@KarishmaGhiya KarishmaGhiya commented Jul 23, 2024

Packages impacted by this PR

@azure/identity

Issues associated with this PR

Describe the problem that is addressed by this PR

  • The token parsing logic in AzurePowershellCredential currently does not account for warmings being emitted by az-powershell, due to which the json token parsing when attempted on the warning string will definitely fail.
  • One option is to suppress the warnings,
  • the other is to make the token parsing logic more robust

We will do both in case we don't want to suppress few warnings (future-proofing solutions)

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

In this PR, I am essentially making the token parsing logic more robust to account for all strings and multiple json objects, carefully pick the correct json for the access token and log the rest of the output as warning with the logger.

Are there test cases added in this PR? (If not, why?)

  • Yes two test cases

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Added a changelog (if necessary)

@KarishmaGhiya KarishmaGhiya requested review from maorleger, schaabs and a team as code owners July 23, 2024 21:51
@KarishmaGhiya KarishmaGhiya changed the title [Identity] Fix PowersehllCredential token parsing logic [Identity] Fix PowershellCredential token parsing logic Jul 23, 2024
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Copy link
Member

@maorleger maorleger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment and a few nits related to tests

@KarishmaGhiya KarishmaGhiya enabled auto-merge (squash) July 25, 2024 22:47
@KarishmaGhiya KarishmaGhiya merged commit 128c4db into Azure:main Jul 26, 2024
14 checks passed
KarishmaGhiya added a commit to KarishmaGhiya/azure-sdk-for-js that referenced this pull request Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants